style(_progress): Provide padding for the vjs-progress-holder while…
#3658
Conversation
… allowing it to resize on hover. Thereby containing the `MouseTimeDisplay` to the `vjs-progress-holder` instead of it bleeding outside. NOTE: Guessing that the margin was put in place to provide some spacing between the slider and the surrounding controls. Because the slider itself, or rather, `vjs-progress-holder` becomes larger on hover, we cannot have the "padding" also be responsive on the same element.
| @@ -3,7 +3,6 @@ | |||
| position: relative; | |||
| cursor: pointer; | |||
| padding: 0; | |||
| margin: 0 0.45em 0 0.45em; | |||
There was a problem hiding this comment.
does the volume slider still look ok?
There was a problem hiding this comment.
It seems like only this removal is necessary to fix the tooltip issue.
| @@ -15,6 +15,7 @@ | |||
| .video-js .vjs-progress-control { | |||
| @include flex(auto); | |||
| @include display-flex(center); | |||
| margin: 0 0.45em 0 0.45em; | |||
There was a problem hiding this comment.
Looks like this would break the Brightcove skin by making the tooltips go out of bounds with the keepTooltipsInside. It probably would also break skins like https://github.com/mmcc/videojs-skin-twitchy.
There was a problem hiding this comment.
I would just get rid of this line as it seems to be unnecessary.
There was a problem hiding this comment.
If there isn't any margin added it makes it very difficult to access the surrounding components because there is only 1px between the volume and the seekbar at position 0.
I didn't quite follow the comment.
There was a problem hiding this comment.
Actually, you're right. There is a slight difference.
The issue is that with this line here, our tooltips look like this:
notice how the right side is a bit cut off. Vs, this:
The position of the mouse marker in both those cases is correct but the tooltip isn't.
There was a problem hiding this comment.
It would seem that the Brightcove skin might need to include that in the source if it utilizes a fullscreen SeekBar?
Should video.js accommodate fullscreen SeekBar in its implementation?
Not sure best way to move forward on this.
There was a problem hiding this comment.
it isnt just in fullscreen that's the issue. It gets cut off by the overflow. The videojs-skin-twichy would also have a similar issue.
But basically it means that we cannot make this change as is because it would break this usecase.
There was a problem hiding this comment.
Hmm, I am not sure how we would go about fixing this issue if the default styles directly conflict with other skins.
Maybe the Brightcove skin is doing something different (Haven't tested).
The default is embedded in the control bar, but the other is not...? I guess its kind of chicken/egg'ish.
If it makes sense for the default implementation to be the primary one, then the other skins would need to undo these changes?
The twitchy skin implements https://github.com/mmcc/videojs-skin-twitchy/blob/master/src/components/_slider.scss#L6 (which removes the inline SeekBar margin). It would also need to remove the .vjs-progress-control ones too (if they were added).
non-modified twitchy skin (has the same out of bounds issue though—might be a different issue?—it breaks regardless of keepTooltipsInside)
Not saying that this should be the case, just that the styles are conflicting—And its weird for the default implementation to have a MouseTimeDisplay that goes out of bounds, especially on the home page :p.
Maybe I should ping in channel and get other ideas?
| @@ -3,7 +3,6 @@ | |||
| position: relative; | |||
| cursor: pointer; | |||
| padding: 0; | |||
| margin: 0 0.45em 0 0.45em; | |||
There was a problem hiding this comment.
It seems like only this removal is necessary to fix the tooltip issue.
|
I think I'm going to close this PR. Not sure exactly how to fix this without potentially breaking users. In videojs 6.0, we're going to transition to only having one kind of progress with mouse tooltips and we can make the fix there. |
Description
Fixes #3645. Looks like it doesn't effect
volume-bar, which is the only other dependent ofSlider, because it manages its own margins. SEE: https://github.com/videojs/video.js/blob/master/src/css/components/_volume.scss#L28Specific Changes proposed
Requirements Checklist
… allowing it to resize on hover. Thereby containing the
MouseTimeDisplayto thevjs-progress-holderinstead of it bleeding outside.NOTE: Guessing that the margin was put in place to provide some spacing between the slider and the surrounding controls. Because the slider itself, or rather,
vjs-progress-holderbecomes larger on hover, we cannot have the "padding" also be responsive on the same element.